Conversation
By the time that `Karafka::App#initialized!` is called,
`Karafka.producer` was already initialized.
On top of that, there's really nowhere we can hook in the Karafka
initialization where we can be sure that `Karafka.producer` wasn't yet
initiaized - that's because the Karafka initialization is NOT
necessarily tied to the WaterDrop initialization. For instance, the
following is a possible scenario:
```
$producer = WaterDrop::Producer.new { ... }
Datadog.configure do |c|
c.tracing.instrument :karafka
end
# note that the producer was initialized before the Karafka app (and in
# this case, even before datadog was configured)
Karafka::App.setup do |c|
c.producer = $producer
end
```
So instead of trying to hook somewhere before `Karafka.producer` is
initialized, let's simply listen to a Karafka after-initialization event
and append our middleware to the producer when that event is triggered.
|
Should we be dropping the waterdrop min version in this PR? |
@ericfirth Not sure about that 🤔 Short answer: I'm fine with that, but this decision has to come from the Datadog team. Longer answer: When I did the initial PR to add the WaterDrop integration I've added support to version dd-trace-rb/appraisal/ruby-3.4.rb Line 147 in 6b654d4 The problem we had before version @mensfeld (Karafka's maintainer) noted the struggle and added class-level notification events on version producer = WaterDrop::Producer.new { |c| c.kafka = {"bootstrap.servers": "redpanda:9092"} }
# (before 2.8.8.rc1) events issued: none
# (since 2.8.8.rc1) events issued: 'producer.created', 'producer.configured'Given that change on WaterDrop, @marcotc reviewed the PR:
I wasn't too sure about the change being requested and argued a bit on my reply to that but, as I did not receive any reply after that (and that Karafka maintainer was personally fine with restricting the compatibility to So, answering whether we should lower the minimum WaterDrop version requirement: we (as in my workplace) don't have a usecase for that and I am not personally interested on it anymore, but if the DataDog team has an interest on it I don't see a problem (and perhaps they could refer to my original changes if that helps). I hope that was clear. Apologies if my tone seemed off a bit - that wasn’t my intention. 🙇 |
Thank you for the detailed reply and so much information. I apologize as I think my question was a bit unclear and maybe I didn't fully understand the PR when I posed it. This particular change, I might have have misunderstood it. The description made it seem like Karafka instrumentation doesn't work (especially with Waterdrop), but I made a test app and saw it work (or at least for DSM which was the part I was concerned with). In that test, the waterdrop version was 2.8.8. So I interpreted this PR as making Waterdrop work with 2.6+ since my understanding was that Waterdrop instrumentation worked. However, reading your comment and then re-reading the description and code more carefully, I now understand why my test worked and what this PR does and doesn't do. I think the previous decision to restrict to recent versions of Waterdrop is fine |
|
Thank you for your contribution and sorry for the delay! We're running the CI here: #5193 The type checking job is failing, please run |
|
Thanks for the feedback @vpellan 🙇 I just added the missing |
Strech
left a comment
Strech
left a comment
There was a problem hiding this comment.
Left one non-blocking suggestion
Drowze
force-pushed
the
fix-karafka-framework
branch
from
1460b88 to
b1fbdf5
Compare
4 participants
What does this PR do?
Fixes the automatic WaterDrop integration activation (via the Karafka integration).
For context, by the time that
Karafka::App#initialized!is called,Karafka.producerwas already initialized.Not only that, there's really nowhere we can hook in the Karafka initialization where we can be sure that
Karafka.producerwasn't yet initiaized - that's because the Karafka initialization is NOT necessarily tied to the WaterDrop initialization. For instance, the following is a possible scenario:So instead of trying to hook somewhere before
Karafka.produceris initialized, let's simply listen to a Karafka after-initialization event and append our middleware to the producer when that event is triggered.Motivation:
We've noticed that
WaterDropdistributed tracing was not working after configuring only the Karafka integration.Change log entry
Additional Notes:
How to test the change?